Skip to content

chore: Add a DA service unit test #3423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 35 commits into from
May 1, 2025
Merged

Conversation

EmandM
Copy link
Collaborator

@EmandM EmandM commented Apr 24, 2025

MTT-11550

Changelog

  • Added: A unit test against the da service

Testing and Documentation

  • Includes unit tests.

Backport

No backport applicable - DA only

@EmandM EmandM requested review from NoelStephensUnity and a team as code owners April 24, 2025 16:10
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 1 or 2 questions and a minor suggestion on wording for API documentation.
Otherwise, very nice job:

  • Cleaning up various areas within the NetcodeIntegrationTest.
    • Improved the "readability" in many places.
    • Added a more standard way to obtain various NetworkManager instances.
  • Cleaning up various integration tests
    • Improved the "readability" in many places.
    • Created a more uniform standard "flow" and naming.
  • Adding UseCMBService to tests that still needed conversion along with comments.
  • Removing m_EnableVerboseDebug to reduce over-all test runner log size.

🥇 💯

EmandM and others added 2 commits April 30, 2025 14:57
This is an attempt to fix an issue where Mac OSX is frequently failing the InterpolationStopAndStartMotionTest. Based on the error, I am thinking (hard to tell since it doesn't replicate locally in-editor nor stand alone) that it could be due to a Mac VM running slower than expected (randomly) which could cause the network tick event to trigger more than once in a single frame.
Copy link
Collaborator

@michalChrobot michalChrobot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done!

# Set this to ensure the DA codec tests will fail if they cannot connect to the echo-server
# The default is to ignore the codec tests if the echo-server fails to connect
ENSURE_CODEC_TESTS: "true"
USE_CMB_SERVICE: "true"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where we are using it 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's inside the UseCMBService() function inside NetcodeIntegrationTest.cs (here)

@EmandM EmandM merged commit 8dc06bf into develop-2.0.0 May 1, 2025
40 checks passed
@EmandM EmandM deleted the chore/rust-unit-test branch May 1, 2025 22:10
NoelStephensUnity added a commit that referenced this pull request May 13, 2025
## This PR Includes

### Initial setup changes
- Minor improvements to the initial CMB server detection and adjustments
to the initial configuration order of operations.
- Session owner now starts and connects ahead of the other clients.

### Ignoring tests
This PR includes updates that will ignore any test that:
- Uses a client-server network topology
- _No point in re-running these as they will have already run multiple
times on multiple platforms before testing against the CMB server._
- Has overridden UseCmbService and is returning false.
- _The test has yet to be converted and so no point in even attempting
to run it._
- _This also includes the `TODO: [CmbServiceTests]` to help track all
tests that need to be converted or reviewed to determine if they need to
be converted._
- Is not derived from NetcodeIntegrationTest.
- _Many of these (not all) don't need to run again, but they all have
the `TODO: [CmbServiceTests]` to help track all tests that need to be
converted or not._
- Does not need to be tested against the CMB Server (i.e. a test for
some kind of functionality without actually starting a session and the
like).

_(This helps to reduce the time to complete the CMB Server integration
tests)_

### Marking/Tracking "for review" tests
Assuring that every test that needs to be reviewed includes the `TODO:
[CmbServiceTests]` using of the two ways:

For classes that derive from `NetcodeIntegrationTest` the existing
pattern used in #3423 was applied:
```
        // TODO: [CmbServiceTests] Adapt to run with the service
        protected override bool UseCMBService()
        {
            return false;
        }
```

For classes that **do not** derive from `NetcodeIntegrationTest`, an
added internal helper method was used:
```
        [OneTimeSetUp]
        public void OneTimeSetup()
        {
            // TODO: [CmbServiceTests] if this test is deemed needed to test against the CMB server then update this test.
            NetcodeIntegrationTestHelpers.IgnoreIfServiceEnviromentVariableSet();
        }
```

_(This PR includes some tests that includes one of the two script
segments but was was reviewed and determined it didn't need to be run
against a CMB server)_

## Fixes
Some issues were exposed in regards to the `NetworkClient` not being set
to approved on all clients relative to each local `NetworkManager`
instance when using the distributed authority network topology and
connecting to a CMB server.

## Changelog

NA - All internal testing and/or specific to testing.

## Testing and Documentation

- Includes integration test adjustments.
- Includes `NetcodeIntegrationTest` related adjustments.
- Includes `NetcodeIntegrationTestHelpers` related adjustments.
- No documentation changes or additions were necessary.

## Backport

Does not require a backport since these changes are specific to updates
in #3423 and over-all distributed authority integration testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants